Skip to content

discovery-index(query): stop caching a GitHub-failure-truncated candidate set for the full TTL and serving it as complete - #10243

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
phamngocquy:miner/issue-10031
Jul 31, 2026
Merged

discovery-index(query): stop caching a GitHub-failure-truncated candidate set for the full TTL and serving it as complete#10243
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
phamngocquy:miner/issue-10031

Conversation

@phamngocquy

Copy link
Copy Markdown
Contributor

Summary

fetchRepoIssues and searchIssues abandon pagination on the first non-OK page and return the pages they
already collected, with a warning (packages/discovery-index/src/github-client.ts:189-203):

    for (let page = 0; url !== null && page < this.maxPages; page += 1) {
      const response: Response = await this.fetchWithRetry(url);
      if (!response.ok) {
        warnings.push(`GitHub returned ${response.status} for ${repoFullName} issues`);
        return { issues, warnings };
      }
      const payload: unknown = await response.json().catch(() => null);
      if (!Array.isArray(payload)) {
        warnings.push(`GitHub returned a non-array issues payload for ${repoFullName}`);
        return { issues, warnings };
      }

searchIssues (packages/discovery-index/src/github-client.ts:214-231) has the identical shape. The caller
logs the warning and otherwise ignores it (packages/discovery-index/src/discovery-query.ts:176-178):

    const { issues, warnings } = await deps.github.fetchRepoIssues(repoFullName);
    surfaceGithubWarnings(repoFullName, warnings);
    for (const issue of issues) addCandidate(repoFullName, issue, verdict);

Two things then compound it:

  1. runDiscoveryQuery caches whatever computeCandidates returned, for the full TTL
    (packages/discovery-index/src/discovery-query.ts:213-216):

      const allCandidates = await deps.resultCache.getOrCompute(scopeKey, deps.cacheTtlMs, () => {
        missed = true;
        return computeCandidates(query, deps);
      });

    A page-2 500 during one cold miss therefore pins a truncated candidate set for DEFAULT_CACHE_TTL_MS
    (300 s, packages/discovery-index/src/discovery-query.ts:40) for every caller sharing that scope. The
    whole point of the service is that many miners share one cached scope, so one transient GitHub failure is
    amplified across the fleet rather than isolated to the request that hit it.

  2. The response then asserts completeness. runDiscoveryQuery emits
    nextCursor: nextOffset < allCandidates.length ? encodeCursor(nextOffset) : null
    (packages/discovery-index/src/discovery-query.ts:218-221), so a client walking the cursor reaches
    nextCursor: null and correctly concludes it has seen the whole set — which it has not. Nothing on the
    wire distinguishes "this scope really has 40 candidates" from "GitHub 500'd on page 2 and we have 40 of
    200". The only signal is a console.error warn line inside the server process.

This is the same class of gap the main app already closed for its own truncated GitHub reads: fetchRepoTree's
consumer treats a truncated tree as inconclusive and returns null rather than a short list
(src/github/migration-tree.ts:41-44), and fetchPullRequestFiles pushes an explicit truncation warning into
the review's own warnings so the PR is held rather than silently evaluated on a partial file set
(src/github/backfill.ts:2469-2476).

Deliverables

  • computeCandidates returns both the candidate list and a completeness flag (or equivalent) derived from
    the warnings already collected at packages/discovery-index/src/discovery-query.ts:176-178, :183-185,
    :190-192.
  • runDiscoveryQuery skips the resultCache write when the pass was incomplete. Exact expectation: with a
    GitHubClientLike stub whose fetchRepoIssues returns { issues: [oneIssue], warnings: ["GitHub returned 500 for owner/repo issues"] },
    two successive runDiscoveryQuery calls for the same query BOTH invoke fetchRepoIssues (i.e. the
    second call is a cache miss), and both responses carry the one candidate.
  • A test in test/unit/discovery-index/discovery-query.test.ts asserting the unchanged happy path: a stub
    returning { issues: [...], warnings: [] } is called exactly once across two successive
    runDiscoveryQuery calls for the same query, and the second response is identical to the first.
  • A test in test/unit/discovery-index/discovery-query.test.ts covering the searchIssues warning path
    (an orgs-scoped query) as well as the fetchRepoIssues path — both feed the same completeness flag and
    a partial fix could cover only one.
  • A regression test at test/unit/discovery-index/discovery-query.test.ts named for this bug (e.g.
    "REGRESSION: a GitHub-failure-truncated candidate set is not cached for the TTL").

All Deliverables above are required in a single PR. A PR that satisfies only some of them — for example one
that threads the flag through computeCandidates but still calls resultCache.getOrCompute, so the truncated
set is written before the flag is ever consulted — does not resolve this issue.

Test plan

This repo enforces 99%+ Codecov patch coverage, branch-counted. vitest.config.ts's coverage.include
covers src/**/*.ts and packages/loopover-engine/src/**/*.tspackages/discovery-index/src/discovery-query.ts
is not in coverage.include, so codecov/patch does not gate this path. The tests above are still
mandatory: test/unit/discovery-index/discovery-query.test.ts already exists and runs in npm run test:ci,
and the deliverables are only verifiable through it.

Both arms of every new branch must be tested: warnings-present vs warnings-empty for the repo path, the same
for the org-search path, the same for the search-terms path, and the cache-write vs cache-skip decision in
runDiscoveryQuery. The getOrCompute short-circuit (cached value present) and the compute path must both
still be exercised.

This change is NOT in packages/loopover-engine/src/**, so the dual-upload engine-coverage rule does not apply.

Fixes #10031

…date set for the full TTL and serving it as complete

Fixes JSONbored#10031
@phamngocquy
phamngocquy requested a review from JSONbored as a code owner July 31, 2026 13:57
@loopover-orb

loopover-orb Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-31 14:06:24 UTC

2 files · 1 AI reviewer · no blockers · readiness 98/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This PR correctly propagates a `complete` flag from `computeCandidates` through to `runDiscoveryQuery`, so a candidate set built from a page that returned GitHub warnings (5xx, non-array payload) is served to the requesting caller but never written into the shared result cache via `deps.resultCache.set`. The fix targets the right layer: rather than patching the retry loop in github-client.ts, it prevents an already-known-incomplete snapshot from being promoted into a cache shared across the fleet. The manual get/compute/set replacement of `getOrCompute` at discovery-query.ts:224-233 is correctly gated on `computed.complete`, and the new tests exercise the real path (a stub GitHub client returning `warnings`), not a fabricated payload shape.

Nits — 5 non-blocking
  • nit: the `complete` flag is only ever set to `false`, never explained for the case where a later loop iteration succeeds after an earlier one failed — worth a one-line comment noting that any single failure poisons the whole scope's cacheability, not just partial per-repo granularity.
  • nit: discovery-query.ts:230 comment references 'listMigrationFilenamesAtRef's truncated-tree posture' — this cross-module analogy is only useful if that pattern actually exists and is discoverable; consider a self-contained rationale instead.
  • nit: the two near-identical 'does not cache org-search / search-term results' tests (discovery-query.test.ts) are useful but slightly duplicative — could be parameterized with `it.each` to reduce repetition.
  • Consider logging a distinct low-cardinality metric (e.g. `discovery_index_result_cache_skipped_incomplete_total`) when `complete` is false and caching is skipped, so operators can see this happening in aggregate rather than only via warn-level logs per repo.
  • The PR body mentions issue discovery-index(query): stop caching a GitHub-failure-truncated candidate set for the full TTL and serving it as complete #10031 per the external brief — confirm that reference is included in the actual PR description sent to GitHub, since the truncated description shown here doesn't show it.

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #10031
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 69 registered-repo PR(s), 19 merged, 0 issue(s).
Contributor context ✅ Confirmed Gittensor contributor phamngocquy; Gittensor profile; 69 PR(s), 0 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: significant
Linked issue satisfaction

Addressed
computeCandidates now returns a completeness flag derived from warnings across fetchRepoIssues/searchIssues calls, and runDiscoveryQuery only writes to deps.resultCache when the pass is complete, otherwise returning the partial set uncached so the next call retries GitHub — matching the required 'inconclusive, not evidence' pattern without touching TtlCache or the wire contract.

Review context
  • Author: phamngocquy
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, Java, Lua, Jupyter Notebook, C, Dockerfile, Shell, Vim Script
  • Official Gittensor activity: 69 PR(s), 0 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Keep the PR focused and include validation evidence before maintainer review.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 31, 2026
@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.60%. Comparing base (79d7e03) to head (90fbc57).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10243      +/-   ##
==========================================
+ Coverage   80.57%   80.60%   +0.03%     
==========================================
  Files         283      284       +1     
  Lines       59095    59195     +100     
  Branches     6996     7026      +30     
==========================================
+ Hits        47614    47714     +100     
  Misses      11188    11188              
  Partials      293      293              
Flag Coverage Δ
backend 100.00% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
packages/discovery-index/src/discovery-query.ts 100.00% <100.00%> (ø)

@loopover-orb loopover-orb Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LoopOver approves — the gate is satisfied and CI is green.

@loopover-orb
loopover-orb Bot merged commit ee1b86e into JSONbored:main Jul 31, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

discovery-index(query): stop caching a GitHub-failure-truncated candidate set for the full TTL and serving it as complete

1 participant